-
Notifications
You must be signed in to change notification settings - Fork 181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new client for Etherscan API v2 #1407
Conversation
Pull Request Test Coverage Report for Build 11614341087Details
💛 - Coveralls |
be3120b
to
854637f
Compare
self.base_api_url = self.BASE_API_V2_URL | ||
|
||
@classmethod | ||
def _get_supported_networks(cls) -> List[Dict[str, Any]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be cached, and also it shouldn't be a private method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this method will be called rarely times and may even add a new chain along the way. It should have no problems without being cached.
It is updated as public method here
item.get("chainid") == str(network.value) for item in supported_networks | ||
) | ||
|
||
def build_url(self, path: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should be at the top of the class, just for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here
@@ -320,7 +320,7 @@ def __init__( | |||
self.request_timeout = request_timeout | |||
|
|||
def build_url(self, path: str): | |||
url = urljoin(self.base_api_url, path) | |||
url = urljoin(self.base_api_url, f"api?{path}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why api is out of the path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the problem is the name of the parameter. I updated it as query
here
) | ||
|
||
def build_url(self, path: str) -> str: | ||
url = urljoin(self.base_api_url, f"v2/api?chainid={self.network.value}&{path}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v2/api
should be path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's the path. I already updated the name of the parameter.
More info about the migration: https://docs.etherscan.io/etherscan-v2/getting-started/v2-quickstart